-
Notifications
You must be signed in to change notification settings - Fork 21.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] Sqlite3 Should create db for missing parent directories as well #51628
[FIX] Sqlite3 Should create db for missing parent directories as well #51628
Conversation
@fatkodima this is ready for review? Please see if we can merge this. |
@@ -23,12 +23,12 @@ def setup | |||
) | |||
end | |||
|
|||
def test_bad_connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test shouldn't be deleted but updated so it still simulate a failure. Of course it's trickier now that mkdir_p
is used.
One way could be to create a temporary directory and change its permissions. e.g.
dir = Dir.mktmpdir
File.chmod(0x000, dir)
db_path = File.join(dir, "db/cant-be-created.db")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byroot, observe the alterations made:
Before:
begin
Dir.mkdir(dirname)
rescue Errno::ENOENT => error
if error.message.include?("No such file or directory")
raise ActiveRecord::NoDatabaseError.new(connection_pool: @pool)
else
raise
end
end
Now:
FileUtils.mkdir_p(dirname)
The removal of the rescue block is notable. Why?
- The likelihood of encountering an
Errno::ENOENT
exception diminishes since the file or directory will now be created. - In scenarios where permission constraints prevent directory or file creation, triggering a failure, an
Errno::EACCES
exception will be raised instead.
Incorporating a failure test, I've implemented a test here activerecord/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb:16
def test_database_should_not_get_created_for_parent_directories_with_missing_permission
dir = Dir.mktmpdir
File.chmod(0x000, dir)
db_path = File.join(dir, "db/cant-be-created.db")
assert_raise Errno::EACCES do
connection = SQLite3Adapter.new(adapter: "sqlite3", database: db_path)
connection.drop_table "ex", if_exists: true
end
end
This test is green on my local machine. And it's strange that in buildkite/rails
CI it's failing. Could you please check here what I am doing wrong?
bin/test test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
Using sqlite3
Run options: --seed 52964
# Running:
..................................................................................
Finished in 0.070632s, 1160.9469 runs/s, 2718.3146 assertions/s.
82 runs, 192 assertions, 0 failures, 0 errors, 0 skips
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, try File.chmod(0x000, dir)
(not FileUtils
).
Also the bubbled exception shouldn't be an errno, we should handle all SyscallError
and wrap them in ActiveRecord::NoDatabaseError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @byroot! I also attempted to use File.chmod(0x000, dir)
.
https://github.com/rails/rails/pull/51628/files#diff-91ca39a63c4ff81c0580c12472a0a709c1d9c428a3ace4970ff3076d90ba6b13
But that doesn't seems to work.
Interestingly, the same test runs fine on my local machine.
I debugged and discovered something unexpected in CI.
https://buildkite.com/rails/rails/builds/106656#018f1f16-c702-468a-9f0b-d14d5eeba392
That parent directory permission mode is: /tmp/d20240427-17-857wvl: 40000
And this parent directory is still able to make the child directorydb
with /tmp/d20240427-17-857wvl/db: 40755
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byroot , I've identified the issue above and why it's exhibiting this unusual behavior.
To replicate the behavior, I conducted independent experiments with the FileUtils.mkdir_p method. Consequently, I crafted a script to test the method:
require 'test/unit'
require 'fileutils'
require 'tmpdir'
require 'debug'
class TestFileUtils < Test::Unit::TestCase
def test_write_independent_test_to_check_fileutils
Dir.mktmpdir do |dir|
# Create a directory with restricted permissions
File.chmod(0o000, dir)
# Attempt to create a directory within the restricted directory
db_path = File.join(dir, "check-fileutils/db/-cinco-dog.sqlite3")
assert_raise Errno::EACCES do
FileUtils.mkdir_p(db_path)
end
end
end
end
When I executed the above script on my local machine with Ruby installed (ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin22]
), it ran without issues:
(base) manishsharma@Manishs-MacBook-Air ~/Development/personal/opensource/experiment ruby reproduce.rb
Loaded suite reproduce
Started
.
Finished in 0.000621 seconds.
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
1 tests, 1 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
1610.31 tests/s, 1610.31 assertions/s
Since Builtkite Rails CI runs on Docker, I decided to run the same script within a Docker container. For this purpose, I created the following Dockerfile:
FROM ruby:latest
COPY reproduce.rb .
ENTRYPOINT ["ruby", "reproduce.rb"]
The reproduce.rb
script remains unchanged.
Here's the output from running and building the image:
(base) manishsharma@Manishs-MacBook-Air ~/Development/personal/opensource/experiment docker run fileutils-reproduce
Loaded suite reproduce
Started
F
===============================================================================
Failure: test_write_independent_test_to_check_fileutils(TestFileUtils): <Errno::EACCES> exception was expected but none was thrown.
reproduce.rb:15:in `block in test_write_independent_test_to_check_fileutils'
12: # Attempt to create a directory within the restricted directory
13: db_path = File.join(dir, "check-fileutils/db/-cinco-dog.sqlite3")
14:
=> 15: assert_raise Errno::EACCES do
16: FileUtils.mkdir_p(db_path)
17: end
18: end
/usr/local/lib/ruby/3.3.0/tmpdir.rb:99:in `mktmpdir'
reproduce.rb:8:in `test_write_independent_test_to_check_fileutils'
===============================================================================
Finished in 0.003941125 seconds.
-------------------------------------------------------------------------------
1 tests, 1 assertions, 1 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
0% passed
-------------------------------------------------------------------------------
253.73 tests/s, 253.73 assertions/s
The question arises: Why does the Docker environment not raise an exception?
Upon inspecting the Docker container, I discovered that chmod
sets the correct permissions for the directory:
# pwd
/tmp
# ls -la
total 16
drwxrwxrwt 1 root root 4096 Apr 27 15:07 .
drwxr-xr-x 1 root root 4096 Apr 27 15:03 ..
d--------- 2 root root 4096 Apr 27 15:03 d20240427-1-ihvptr
d--------- 3 root root 4096 Apr 27 15:09 d20240427-24-sa4xz6
Creating another directory inside d20240427-24-sa4xz6
occurs without errors because both the owner and group have root privileges:
# cd d20240427-24-sa4xz6
# mkdir -p create-without-issue
# ls -la
total 16
d--------- 4 root root 4096 Apr 27 15:27 .
drwxrwxrwt 1 root root 4096 Apr 27 15:07 ..
drwxr-xr-x 2 root root 4096 Apr 27 15:27 create-without-issue
drwxr-xr-x 2 root root 4096 Apr 27 15:09 temp
In summary, the root user can create files in a directory marked as Read Only.
Confirmed the user in Builkite Rails CI pipeline as well it's a root user:
https://buildkite.com/rails/rails/builds/106668#018f2402-28f6-4a89-a27d-04b675b70ed6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byroot , did you have a chance to look at it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried solution:
- Make directories immutable (using chattr or chflags)
with chattr command it gives an error:
https://buildkite.com/rails/rails/builds/106846#018f48ea-55a2-4af2-9f8d-acf4c38f1a57
chattr: Operation not permitted while setting flags on <directory-name>
Outcome: Docker Deny access to some filesystem operations, like creating new device nodes, changing the owner of files, or altering attributes (including the immutable flag).
It's mentioned here: https://docs.docker.com/engine/security/#linux-kernel-capabilities
Solution to solve this is: This is related to capabilities thing: chattr requires CAPLINUX IMMUTABLE which is disabled in docker by default. Just add -- cap-add
LINUX_ IMMUTABLE to docker container start options to enable it.
That's itself another issue on. I don't know whether to proceed with this or not as we have to do modification in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, got side tracked, looking at it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I think that the issue is CI runs as root, so it's hard to simulate any problem into creating a directory or file.
So while I don't like not covering this sort of things with a test, I don't quite see what we could do, and anyway it could likely fail in dozens of different ways, so coverage here will never be that good.
I'll remove the test, sorry for sending you on a goose chase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! @byroot. Goose chase helped me go dig deep into docker and linux.
0b3b264
to
32d13c5
Compare
844d985
to
45aa5e4
Compare
45aa5e4
to
1d72f53
Compare
1d72f53
to
fd94a12
Compare
Motivation / Background
This Pull Request has been created to fix #51623
ActiveRecord::ConnectionAdapters::SQLite3Adapter#initialize will check if a sqlite3 database file path exists, and if not it will attempt to create the directory using Dir.mkdir. However, Dir.mkdir cannot create all parent directories like FileUtils.mkdir_p. Thus an No such file or directory @ dir_s_mkdir - does/not/exist/yet (Errno::ENOENT) will be raised if one of the parent-parent directories of the sqlite3 database file do not exist yet.
Detail
This Pull Request changes:
ActiveRecord::ConnectionAdapters::SQLite3Adapter#initialize
Replaced the
Dir.mkdir(dirname)
withFileUtils.mkdir_p(dirname)
to create the directory for the parent directories in the path if they don't exists.Additional information
Checklist
Before submitting the PR make sure the following are checked:
[Fix #51623 ]